Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a ProducerConsumer helper for processing a sequence of items in parallel. #73323

Merged
merged 12 commits into from
May 3, 2024

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2024
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 3, 2024 05:52
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 3, 2024 05:52
@@ -222,23 +222,20 @@ private static bool IsHostOrRemoteWorkspace(Project project)

// Defer to the ProducerConsumer. We're search the unreferenced projects in parallel. As we get results, we'll
// add them to the 'allSymbolReferences' queue. If we get enough results, we'll cancel all the other work.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view with whitespace off.

}),
await ProducerConsumer<ImmutableArray<SymbolReference>>.RunParallelAsync(
viableUnreferencedProjects,
produceItems: static async (project, onItemsFound, args) =>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this form is much easier to understand (for me at least). instead of hte nested lambdas, you just pass hte single lambda. It will be called in parallel, passing a single item from "viableUnreferencedProjects", and teh callback to call when items are produced based on htat item.

@CyrusNajmabadi CyrusNajmabadi changed the title Provider a ProducerConsumer helper for processing a sequence of items in parallel. Provide a ProducerConsumer helper for processing a sequence of items in parallel. May 3, 2024
{
return RunAsync(
// We're running in parallel, so we def have multiple writers
ProducerConsumerOptions.SingleReaderOptions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProducerConsumerOptions.SingleReaderOptions

Why not let the caller specify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind, I can see clearly now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-D

@@ -42,21 +42,21 @@ internal static class ProducerConsumer<TItem>
/// </summary>
public static Task RunAsync<TArgs>(
ProducerConsumerOptions options,
Func<Action<TItem>, TArgs, Task> produceItems,
Func<ImmutableArray<TItem>, TArgs, Task> consumeItems,
Func<Action<TItem>, TArgs, CancellationToken, Task> produceItems,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

produceItems

naming nit: maybe produceItem instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like produceItems. It's the callback responsible for producing all the items. It will just do that by producing them and passing them to callback it itself is provided.

return RunAsync(
// We're running in parallel, so we def have multiple writers
ProducerConsumerOptions.SingleReaderOptions,
produceItems: static (callback, args, cancellationToken) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

produceItems: static (callback, args, cancellationToken) =>

I think this might be one level too deep for me to understand without having had more coffee. Local function for the producer here might make things clearer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have coffee! I can also extract into local functions on a follow-up if that's ok.

@@ -90,13 +90,13 @@ private static IEnumerable<T> Prioritize<T>(IEnumerable<T> items, Func<T, bool>
/// </summary>
private static Task PerformParallelSearchAsync<T>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PerformParallelSearchAsync

Curious if this method provides any value now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can consider removing in follow up

@CyrusNajmabadi
Copy link
Member Author

@ToddGrun can I make those changes in follow-up?

@CyrusNajmabadi CyrusNajmabadi merged commit d01b164 into dotnet:main May 3, 2024
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the parallelHelper branch May 3, 2024 16:07
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 3, 2024
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants